Skip to content

Selection on choropleth to work even if an invisible trace is present #2099

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Oct 18, 2017
Merged

Selection on choropleth to work even if an invisible trace is present #2099

merged 5 commits into from
Oct 18, 2017

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Oct 18, 2017

Box select and lasso to work in the presence of an invisible choropleth trace. Updated test case signals if the small fix is commented out.

@monfera monfera self-assigned this Oct 18, 2017
@monfera monfera added status: reviewable bug something broken labels Oct 18, 2017
alexcjohnson
alexcjohnson reviewed Oct 18, 2017
View reviewed changes
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable - but I wonder if we should preempt problems like this by just filtering out invisible traces (visible !== true, to account for legendonly) here?

@monfera
Copy link
Contributor Author

monfera commented Oct 18, 2017

Thanks @alexcjohnson, I understand the general direction for preventing the situation from emerging, not sure how I'd go about it as node3 etc. are handled outside the scope of selection. I'd need to urgently work on table.restyle but circle back to this ASAP.

@alexcjohnson
Copy link
Collaborator

@monfera the idea is that the only reason node3 would not be set is if the trace is not visible. So if we filter invisible traces out before even calling selectPoints, we don't have to worry about this kind of error in any of the individual modules.

@etpinard
Copy link
Contributor

but I wonder if we should preempt problems like this by just filtering out invisible traces (visible !== true, to account for legendonly)

This is a much better solution in my opinion.

@monfera monfera mentioned this pull request Oct 18, 2017
Closed
etpinard
etpinard reviewed Oct 18, 2017
View reviewed changes
src/plots/cartesian/select.js Outdated
@@ -190,6 +195,7 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) {
selection = [];
for(i = 0; i < searchTraces.length; i++) {
searchInfo = searchTraces[i];
if(!visible(searchInfo)) continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Does that mean we can 🔪 those visible !== false clauses here, here, here, here and here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find, indeed these are the only callers, and select check is almost always on the top, except scattermapbox/select/js where it's okay to set the _hasDimmedPts to false anyway:

    trace._hasDimmedPts = false;

    if(!subtypes.hasMarkers(trace)) return [];

Pushing a new commit now

@monfera
Copy link
Contributor Author

monfera commented Oct 18, 2017

moved check more upstream on Alex's suggestion

@etpinard
Copy link
Contributor

💃 for me. Great PR @monfera !

@monfera
Copy link
Contributor Author

monfera commented Oct 18, 2017

Thanks @etpinard and @alexcjohnson for the added clarity and code size reducing suggestions!

@monfera monfera merged commit fff9c6e into master Oct 18, 2017
@monfera monfera deleted the invisible-select branch October 18, 2017 23:19
etpinard added a commit that referenced this pull request Oct 20, 2017
@etpinard
fixup choropleth select test
1760d66 
- PR #2099 got merged
  on a branch behind #2081
  which caused the test to fail on master.
@etpinard etpinard mentioned this pull request Oct 20, 2017
Merged
etpinard added a commit that referenced this pull request Oct 23, 2017
@etpinard
fixup choropleth select test
6b44d39 
- PR #2099 got merged
  on a branch behind #2081
  which caused the test to fail on master.
@etpinard etpinard mentioned this pull request Oct 23, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcjohnson alexcjohnson alexcjohnson left review comments

@etpinard etpinard etpinard left review comments

Assignees

@monfera monfera

Labels
bug something broken
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@monfera @alexcjohnson @etpinard